NOTE: I suggest we don't merge this (or at least we don't release a new version) until tests are added and CI jobs running those tests in different archs and Node versions. Note for example that the previous server.bind() and server.listen() usages in udp.js and tcp.js were wrong.

I suggest we don't merge this (or at least we don't release a new version) until tests are added and CI jobs running those tests in different archs and Node versions.

Makes sense.

When I said that we should add tests here I meant "We The Team".

When I said that we should add tests here I meant "We The Team".

Yep, I'm currently working on few unpopular and time consuming tasks. I may jump here when I find the time.

BTW, if tests and CI was a blocker, IMHO it should have been done before this PR to avoid this block.

Did you notice that this lib was using the server.bind() and server.listen() incorrectly and maybe it was not binding in the wrongly given second address argument or maybe it worked by accident? I'll confirm that later and comment here.

So code in master branch looks like follows in tcp.js and udp.js files (simplified):

TCP

const net = require('net');

const server = net.createServer();

const port = 1234;
const ip = '127.0.0.1';

server.listen({ port, exclusive: true }, ip, () => console.log('done'));

Let's run it, let it running and check port 1234 usages in the system:

$ sudo lsof -i -n -P | grep 1234

node      71714             ibc   15u  IPv6 0x2efae2ae2f13914b      0t0    TCP *:1234 (LISTEN)

You see? It's listening in TCP *:1234 AKA 0.0.0.0:1234 instead than in 127.0.0.1:1234 as expected.

UDP

const dgram = require('dgram');

const server = dgram.createSocket('udp4');

const port = 1234;
const ip = '127.0.0.1';

server.bind({ port, exclusive: true }, ip, () => console.log('done'));

Let's run it, let it running and check port 1234 usages in the system:

$ sudo lsof -i -n -P | grep 1234

node      72054             ibc   15u  IPv4 0x2efae2a4985b720b      0t0    UDP *:1234

You see? It's binding in UDP *:1234 AKA 0.0.0.0:1234 instead than in 127.0.0.1:1234 as expected.

Why?

Because the API usage is wrong.

• See all signatures of server.listen(): https://nodejs.org/api/net.html#serverlisten
• There is NO signature in which port is given in an object in first argument and IP is given as string in second argument.
• See all signatures of server.bind(): https://nodejs.org/api/dgram.html#socketbindport-address-callback
• There is NO signature in which port is given in an object in first argument and IP is given as string in second argument.

Now let's try with the fixed code in this PR:

TCP

import * as net from 'node:net';

const server = net.createServer();

const port = 1234;
const ip = '127.0.0.1';

server.listen({ port, host: ip, exclusive: true }, () => console.log('done'));

$ sudo lsof -i -n -P | grep 1234

node      75323             ibc   16u  IPv4 0x2efae2ae31fa6713      0t0    TCP 127.0.0.1:1234 (LISTEN)

UDP

import * as dgram from 'node:dgram';

const server = dgram.createSocket('udp4');

const port = 1234;
const ip = '127.0.0.1';

server.bind({ port, address: ip, exclusive: true }, () => console.log('done'));

$ sudo lsof -i -n -P | grep 1234

node      75989             ibc   15u  IPv4 0x2efae2a4985a660b      0t0    UDP 127.0.0.1:1234

Tests (WIP, wanna add more) and CI job added: https://github.com/ibc/pick-port/actions/runs/7462140637

We should start using native Node tests rather than Jest. I think the changes needed here would be very minimal.

We should start using native Node tests rather than Jest. I think the changes needed here would be very minimal.

We want this library to work with Node 16 or at least Node 18.

We want this library to work with Node 16 or at least Node 18.

Users may run an old Node version. It's just tests which would require a newer one. 99% of users won't run the tests, and the remaining 1% would have to use a newer Node to do it.

I'm not blocking this PR (it's already approved), but the earlier we start porting tests to Node, the less painful it will be.

Users may run an old Node version. It's just tests which would require a newer one. 99% of users won't run the tests, and the remaining 1% would have to use a newer Node to do it.

We want that tests run in CI hosts using different Node versions, otherwise the purpose of tests is useless considering that we want to support Node >= 16 as declared in package.json.

We want that tests run in CI hosts using different Node versions, otherwise the purpose of tests is useless considering that we want to support Node >= 16 as declared in package.json.

Fair enough.